Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a replace-type parameter to allow replacing package and/or type names #540

Merged
merged 15 commits into from
Mar 20, 2023

Conversation

RangelReale
Copy link
Contributor

Description

Add a replace-type parameter to allow replacing package and/or type names.

This overcomes the problem of type aliases to internal packages, like cloud.google.com/go/pubsub.Message, which is a type alias, for which the mock forwarded the implementation to an internal package that's not exported.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Version of Golang used when building/testing:

  • 1.11
  • 1.12
  • 1.13
  • 1.14
  • 1.15
  • 1.16
  • 1.17
  • 1.18
  • 1.19
  • 1.20

How Has This Been Tested?

Tests included on the MR.
Tested with local code that imported cloud.google.com/go/pubsub.Message.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@LandonTClipp
Copy link
Collaborator

LandonTClipp commented Feb 9, 2023

So I might be missing something here, but it does seem to me like the parsing packages Golang provides would indeed be able to differentiate type alias from the underlying type, as shown by my proof of concept in the linked issue. Although I wonder if the issue is that the types.TypeName passed to getPackageScopedType was resolved to the real type farther up the call stack.

The types package clearly understands the difference between alias and types: https://pkg.go.dev/go/types#TypeName

The question becomes, is the golang.org/x/tools/go/packages package not loading alias info? What led you to believe that the parser doesn't support alias information?

If it is possible to extract alias names, I would much prefer that solution than having the burden be placed on users to create manual exceptions to type names. I have a suspicion mockery is simply doing something wrong here.

@RangelReale
Copy link
Contributor Author

So I might be missing something here, but it does seem to me like the parsing packages Golang provides would indeed be able to differentiate type alias from the underlying type, as shown by my proof of concept in the linked issue. Although I wonder if the issue is that the types.TypeName passed to getPackageScopedType was resolved to the real type farther up the call stack.

The types package clearly understands the difference between alias and types: https://pkg.go.dev/go/types#TypeName

The question becomes, is the golang.org/x/tools/go/packages package not loading alias info? What led you to believe that the parser doesn't support alias information?

If it is possible to extract alias names, I would much prefer that solution than having the burden be placed on users to create manual exceptions to type names. I have a suspicion mockery is simply doing something wrong here.

I looked at the issues of other mock generator and they all have the same problem, see these 2 gomock issues for example:

mockgen should not follow type aliases
Document why a user should choose the source or reflect mode

Looks like they created an alternative "source" mode to parse the source directly to avoid missing this information. I stepped on the code and looked at the object returned to the type alias, I didn't find any mention of the alias anywhere, the same that is said on the first issue above.

So with the current mockery version it is effectivelly impossible to overcome the cloud.google.com/go/pubsub.Message problem, so this is an alternative, as this other implementation will be hard and time-consuming to do.

@LandonTClipp
Copy link
Collaborator

LandonTClipp commented Feb 9, 2023

What I would like to determine before committing to this proposal is what part of the mockery call stack do we lose the alias information? I'm betting that the breakage is happening somewhere in this method:

func (g *Generator) populateImports(ctx context.Context) {

but I want to confirm for certain that there's nothing we could do here. At the moment it's not clear to me at what point the alias information is lost. I'm sure we could add a bunch of print statements in the code to prove it... it would help my sanity.

@RangelReale
Copy link
Contributor Author

What I would like to determine before committing to this proposal is what part of the mockery call stack do we lose the alias information? I'm betting that the breakage is happening somewhere in this method:

func (g *Generator) populateImports(ctx context.Context) {

but I want to confirm for certain that there's nothing we could do here. At the moment it's not clear to me at what point the alias information is lost. I'm sure we could add a bunch of print statements in the code to prove it... it would help my sanity.

It is easy to step in the code using this MR to trigger this problem, step into the generator_test.go:TestInternalPackagePrologue function and you can see what happens to the *types.TypeName variable, I looked at it into a debugger and found no way to go back the the alias value itself.

@wyattanderson
Copy link

So I might be missing something here, but it does seem to me like the parsing packages Golang provides would indeed be able to differentiate type alias from the underlying type, as shown by my proof of concept in the linked issue.

There's a gap in your proof-of-concept. You're correct in that you can identify a type alias declaration. However, aliases are not preserved in the type graph, so any variables, struct fields, etc. of an alias type are resolved as the aliased type itself and any useful information about the alias itself is discarded and unavailable from those nodes. You can't distinguish between the alias and the type to which it refers when using packages.Load, etc., because the information simply isn't there. This, unfortunately, impacts numerous tools in the Go ecosystem beyond just Mockery.

This is the closest thing to an upstream tracking issue for this problem that I can find: golang/go#44410

I'd love to have the proposed functionality as an option until the Go language itself fixes the underlying issue, and I'm not holding my breath waiting for that to happen.

@LandonTClipp
Copy link
Collaborator

LandonTClipp commented Feb 21, 2023

@wyattanderson that makes more sense. I understand the need for this a little better. It's unfortunate and I really hate having to maintain hacky stuff like this but I can't think of a better solution until the go maintainers fix the parsers (which probably isn't anywhere high on their priority list).

After having given it more thought I think we can go ahead with this. However @RangelReale could you add a section in the mkdocs page here: https://vektra.github.io/mockery/features/ . I am not adding any documentation into the root readme anymore. We can just add another section regarding type aliases and how to resolve it with the feature you've implemented here.

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Patch coverage: 90.38462% and project coverage change: +0.42828 🎉

Comparison is base (c96741b) 74.92926% compared to head (ec95f73) 75.35754%.

Additional details and impacted files
@@                 Coverage Diff                 @@
##              master        #540         +/-   ##
===================================================
+ Coverage   74.92926%   75.35754%   +0.42828%     
===================================================
  Files              7           7                 
  Lines           1767        1818         +51     
===================================================
+ Hits            1324        1370         +46     
- Misses           358         362          +4     
- Partials          85          86          +1     
Impacted Files Coverage Δ
pkg/config/config.go 71.17117% <ø> (ø)
pkg/outputter.go 33.89831% <0.00000%> (-0.19260%) ⬇️
pkg/generator.go 93.22709% <91.83673%> (-0.10624%) ⬇️
cmd/mockery.go 64.32927% <100.00000%> (+0.10908%) ⬆️
pkg/walker.go 64.86486% <100.00000%> (+0.31940%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@RangelReale
Copy link
Contributor Author

@LandonTClipp yes I fully agree with you, I hate having to add these kinds of workarounds, but in this case there seems to be absolutely no way around it. I'll look at cleaning up the code and updating the documentation tomorrow.

@wyattanderson @LandonTClipp maybe we can detect that this is an alias and issue a warning about it?

@RangelReale
Copy link
Contributor Author

I refactored the parameter parsing to be done only at startup, see if everything is ok.

@LandonTClipp
Copy link
Collaborator

Great work, I’ll give it a review in the next day or so.

Copy link
Collaborator

@LandonTClipp LandonTClipp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some changes I want to make, one of which will hopefully simplify the unit test a bit. Let me know what you think of those.

@@ -2411,6 +2411,93 @@ import mock "github.com/stretchr/testify/mock"
s.checkPrologueGeneration(generator, expected)
}

func (s *GeneratorSuite) TestInternalPackagePrologue() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename this "TestReplaceTypePackagePrologue"

mock.Mock
}

// DoFoo provides a mock function with given fields:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to move away from this type of test where we assert exactly the code that is generated... it's fragile because if you make one change to the mock generation then you have to modify dozens of tests.

Maybe a better way to do this is to write the mock in-memory and simply assert that *baz.Baz exists and *foo.InternalBaz does not exist in the generated string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a regexp check function and used it for this assertion.

@LandonTClipp
Copy link
Collaborator

Thank you sir, I'll take another look at this in the next day or so!

@LandonTClipp LandonTClipp merged commit e964002 into vektra:master Mar 20, 2023
@papadeltasierra
Copy link

Very late to the game but hit this error and I have a problem that this does not solve. I have multiple packages that mockery maps down to a single internal package and I cannot use this parameter to replace back to those packages. So what happens is:

  • mockery follows packages A, B, C to internal package Z
  • I can use replace-type to map Z to A, or B, or C but not all of them!
    The package that causes this is not mine and I cannot fix it so seem unable to use mockery to mock my interface.

@LandonTClipp
Copy link
Collaborator

@papadeltasierra thats really tricky. Unfortunately the only way you can get around this is by manually specifying the replacements for each individual type. There is no way mockery can know where your type alias originally came from because historically, a type alias is not represented in the type tree.

There is a lead I'm chasing up in Go 1.22 where we might be able to retain the alias information if you set a specific GODEBUG flag. The Go authors know this is an issue and have made some half attempt at providing a fix.

@papadeltasierra
Copy link

@LandonTClipp thanks for the info. Currently I've manually "patched" the mockery output and removed this interface from .mockery.yaml. Is there a "very high level for the rest of us" explanation of why mockery even follows down the import trees? Doesn't the signature of the interface functions together with the imports in the interface file tell mockery everything it needs to know?

@LandonTClipp
Copy link
Collaborator

@papadeltasierra yes the high level explanation is that prior to Go 1.22, the parsed type tree did not have any representation for a type alias. It always got resolved down to the underlying type, so there was no way to extract the alias information. This was a limitation with the Go package parser itself, not with mockery.

You can see in 1.22 they added a types.Alias type that would get returned when gotypelias is set to 1. This is probably the solution moving forward, but we need to add support for this in mockery.

@LandonTClipp
Copy link
Collaborator

@papadeltasierra update on the gotypelias lead, we will not be able to get support for handling this cleanly until at least Go 1.23 when the Go devs make the requisite changes to golang.org/x/tools/go/packages. See more details here: #763 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mockery follows type aliases and cannot compile mock
4 participants